Skip to content

Progress reporting improvements #1784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 18 commits into from
May 3, 2021
Merged

Progress reporting improvements #1784

merged 18 commits into from
May 3, 2021

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented May 2, 2021 •
edited
Loading

Rework of #1770. Summary of changes:

  1. progress reporting logic extracted into its own module
  2. Removed one layer of async handling, reducing the chances of race conditions
  3. Correctly implement the LSP spec for window/workDoneProgress/create by waiting until the client has responded
  4. Fix bugs: division by zero (when todo is 0) and handling when NoProgress
  5. Performance: the reporting loop is now O(1) instead of O(N) in the number of files

I think this is the best we can do to close #1749 while sharing the same logic for both tests and app code

@pepeiborra pepeiborra mentioned this pull request May 2, 2021
Closed
@pepeiborra pepeiborra requested a review from wz1000 May 2, 2021 10:50
@pepeiborra pepeiborra force-pushed the progressreport-redo branch from b3ba386 to 793be94 Compare May 2, 2021 10:56
@pepeiborra pepeiborra requested a review from ndmitchell May 2, 2021 10:57
@pepeiborra pepeiborra marked this pull request as ready for review May 2, 2021 14:36
ndmitchell
ndmitchell approved these changes May 3, 2021
View reviewed changes
Copy link
Collaborator

@ndmitchell ndmitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a few updates to the test - are these fixing race conditions that this patch showed up?

This code is way clearer!

@pepeiborra
Copy link
Collaborator Author

I see a few updates to the test - are these fixing race conditions that this patch showed up?

I fixed a bunch of tests in the previous version of this PR and decided to preserve the fixes. Not necessarily race conditions: often tests rely on sketchy invariants, e.g. a test that waits for three logging notifications before checking if the build session is restarted is going to break for reasons unrelated to the property it's checking.

@pepeiborra pepeiborra force-pushed the progressreport-redo branch from 7c40949 to 7fc92b1 Compare May 3, 2021 08:01
@pepeiborra pepeiborra force-pushed the progressreport-redo branch from 7fc92b1 to 3850495 Compare May 3, 2021 08:13
@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label May 3, 2021
@mergify mergify bot merged commit 6ecf17b into master May 3, 2021
@pepeiborra pepeiborra mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndmitchell ndmitchell ndmitchell approved these changes

@wz1000 wz1000 Awaiting requested review from wz1000 wz1000 is a code owner

Assignees
No one assigned
Labels
merge me Label to trigger pull request merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Progress messages logic might have race conditions
2 participants
@pepeiborra @ndmitchell